feat: implement search bar inside of the settings#1686
feat: implement search bar inside of the settings#1686janburzinski wants to merge 4 commits intogeneralaction:mainfrom
Conversation
Implement search functionality for settings with: - Search input component - Search results overlay - useSettingsSearch hook - Updated all settings cards to support search indexing
Collapse keywords/synonyms into a single aliases array, drop unused category/matches fields and the duplicated getHighlightSegments helper, and replace the multi-tier similarity + word fallback with a flat exact/startsWith/includes scorer. Behaviour for normal queries is unchanged; edge cases (special chars, 1-char queries) are more predictable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@janburzinski is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded stable DOM id attributes to many settings UI components; introduced a settings search feature (input, results, search index, and wiring into SettingsPage) and added a typed integration registry used by IntegrationsCard. Tests validate registry/index consistency. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsPage
participant SearchInput as SettingsSearchInput
participant SearchIndex as useSettingsSearch
participant SearchResults as SettingsSearchResults
participant DOM
User->>SearchInput: types query
SearchInput->>SettingsPage: onQueryChange(query)
SettingsPage->>SearchIndex: searchSettings(query)
SearchIndex-->>SettingsPage: SearchResult[]
SettingsPage->>SearchResults: render(results, query)
SearchResults->>User: display grouped results
User->>SearchResults: click result
SearchResults->>SettingsPage: onResultClick(tabId, elementId)
SettingsPage->>SettingsPage: set active tab, clear search
SettingsPage->>DOM: scrollIntoView(elementId)
DOM-->>User: scroll to setting
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
now that i look at it another idea where to put it: inside of the settings sidebar |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/test/renderer/settingsSearchIndex.test.ts (1)
20-27: Consider adding a failure message for misrouted integrations.Current assertion is correct; a message would make CI failures faster to diagnose.
🔎 Optional diagnostics improvement
- expect(misrouted).toEqual([]); + expect( + misrouted, + `Integration entries must use tabId="integrations". Misrouted ids: ${misrouted + .map((entry) => entry.id) + .join(', ')}` + ).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/renderer/settingsSearchIndex.test.ts` around lines 20 - 27, Add an explicit failure message when misrouted integrations are found: after computing misrouted (from INTEGRATION_REGISTRY and SETTINGS_INDEX) add a guard like if (misrouted.length > 0) throw new Error(`Misrouted integrations: ${misrouted.map(m => `${m.id} (expected tabId=integrations)`).join(', ')}`) so CI shows the specific IDs and details before the existing expect check in the test that routes entries to the integrations tab.src/renderer/components/SettingsSearchInput.tsx (1)
4-6: Import statement ordering is unconventional.The
isMacconstant is declared between import statements, which breaks the typical import-first convention. Consider moving it after all imports:♻️ Suggested reordering
import React, { forwardRef } from 'react'; import { Search, X } from 'lucide-react'; - -const isMac = typeof navigator !== 'undefined' && /mac|iphone|ipad|ipod/i.test(navigator.platform); import { Input } from '@/components/ui/input'; import { Button } from '@/components/ui/button'; + +const isMac = typeof navigator !== 'undefined' && /mac|iphone|ipad|ipod/i.test(navigator.platform);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsSearchInput.tsx` around lines 4 - 6, Move the top-level isMac constant declaration so all import statements come first; specifically, relocate the line that defines isMac (using navigator.platform and the /mac|iphone|ipad|ipod/i test) to appear after the imports of Input and Button in SettingsSearchInput.tsx, preserving the same expression and any usages of isMac elsewhere in that file.src/renderer/components/SettingsSearchResults.tsx (2)
8-16: Consider sharing TAB_LABELS with SettingsPage.
TAB_LABELSduplicates the tab label mapping that exists implicitly inSettingsPage.tsx(in thetabsarray). If tab names change, they'd need to be updated in multiple places.This is minor since the labels are unlikely to change frequently and the duplication is small.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsSearchResults.tsx` around lines 8 - 16, TAB_LABELS in SettingsSearchResults.tsx duplicates the tab-to-label mapping already defined in SettingsPage's tabs array; to avoid drift, export the canonical labels from SettingsPage (or a shared module) and import them into SettingsSearchResults instead of maintaining TAB_LABELS. Update SettingsPage to expose the mapping (e.g., export const TAB_LABELS or export the tabs array) and replace the local TAB_LABELS usage in SettingsSearchResults.tsx with an import of that exported symbol so both components read the single source of truth.
60-66: Empty state could benefit from sanitizing the displayed query.The
queryis rendered directly in the empty state message. If the query contains special characters or is very long, this could cause display issues. Consider truncating long queries:♻️ Optional: Truncate long queries in empty state
return ( <div className="flex flex-col items-center justify-center py-20 text-center"> - <p className="text-sm text-muted-foreground">No settings found for "{query}"</p> + <p className="text-sm text-muted-foreground"> + No settings found for "{query.length > 50 ? `${query.slice(0, 50)}…` : query}" + </p> </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsSearchResults.tsx` around lines 60 - 66, The empty-state renders the raw query in SettingsSearchResults, which can be too long or contain problematic characters; add a small helper (e.g., truncateQuery(query: string, max = 50): string) and use it when rendering the <p> so the displayed string is shortened with an ellipsis if over the limit (and continue to rely on React's text rendering for escaping); update the JSX in SettingsSearchResults where it currently renders `No settings found for "{query}"` to use the truncated/sanitized value (e.g., truncated = truncateQuery(query) and render `No settings found for "{truncated}"`).src/renderer/components/IntegrationsCard.tsx (1)
558-561: Type assertion is safe but could be stricter.The
entry.id as IntegrationIdassertion works becauseINTEGRATION_REGISTRYdefines all valid IDs. However, if a new integration is added to the registry without adding a corresponding entry tointegrationRuntime, this would compile but fail at runtime with undefined properties.Consider adding a compile-time check or runtime guard:
♻️ Optional: Add runtime safety check
const integrations = INTEGRATION_REGISTRY.map((entry) => ({ ...entry, - ...integrationRuntime[entry.id as IntegrationId], + ...integrationRuntime[entry.id as IntegrationId] ?? (() => { + console.warn(`Missing runtime config for integration: ${entry.id}`); + return { logoSvg: '', connected: false, loading: false, onConnect: () => {}, onDisconnect: () => {} }; + })(), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/IntegrationsCard.tsx` around lines 558 - 561, The code maps INTEGRATION_REGISTRY and force-casts entry.id to IntegrationId which can mask missing keys in integrationRuntime; update the mapping to explicitly guard or assert presence of a runtime entry: for each entry from INTEGRATION_REGISTRY, lookup integrationRuntime[entry.id] into a local (e.g., runtime) and if absent either throw a clear error or merge with safe defaults before spreading into integrations; alternatively tighten types so INTEGRATION_REGISTRY entries are constrained to keys of integrationRuntime (use a Record or TypeScript's satisfies/const patterns) so missing runtimes are a compile-time error—apply this change around the integrations construction where INTEGRATION_REGISTRY, integrationRuntime, IntegrationId, and integrations are referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/SettingsPage.tsx`:
- Around line 101-139: SETTINGS_INDEX contains individual integration IDs
(github, gitlab, jira, linear, plain, forgejo, sentry) but SETTING_ELEMENT_IDS
lacks mappings for those keys, so clicking an integration search result switches
to the Integrations tab without scrolling; update SETTING_ELEMENT_IDS to include
entries for "github", "gitlab", "jira", "linear", "plain", "forgejo", and
"sentry" and map each to 'integrations-card' so the useSettingsSearch scroll
logic finds a valid elementId. Ensure you modify the exported
SETTING_ELEMENT_IDS constant in SettingsPage.tsx to add these keys.
---
Nitpick comments:
In `@src/renderer/components/IntegrationsCard.tsx`:
- Around line 558-561: The code maps INTEGRATION_REGISTRY and force-casts
entry.id to IntegrationId which can mask missing keys in integrationRuntime;
update the mapping to explicitly guard or assert presence of a runtime entry:
for each entry from INTEGRATION_REGISTRY, lookup integrationRuntime[entry.id]
into a local (e.g., runtime) and if absent either throw a clear error or merge
with safe defaults before spreading into integrations; alternatively tighten
types so INTEGRATION_REGISTRY entries are constrained to keys of
integrationRuntime (use a Record or TypeScript's satisfies/const patterns) so
missing runtimes are a compile-time error—apply this change around the
integrations construction where INTEGRATION_REGISTRY, integrationRuntime,
IntegrationId, and integrations are referenced.
In `@src/renderer/components/SettingsSearchInput.tsx`:
- Around line 4-6: Move the top-level isMac constant declaration so all import
statements come first; specifically, relocate the line that defines isMac (using
navigator.platform and the /mac|iphone|ipad|ipod/i test) to appear after the
imports of Input and Button in SettingsSearchInput.tsx, preserving the same
expression and any usages of isMac elsewhere in that file.
In `@src/renderer/components/SettingsSearchResults.tsx`:
- Around line 8-16: TAB_LABELS in SettingsSearchResults.tsx duplicates the
tab-to-label mapping already defined in SettingsPage's tabs array; to avoid
drift, export the canonical labels from SettingsPage (or a shared module) and
import them into SettingsSearchResults instead of maintaining TAB_LABELS. Update
SettingsPage to expose the mapping (e.g., export const TAB_LABELS or export the
tabs array) and replace the local TAB_LABELS usage in SettingsSearchResults.tsx
with an import of that exported symbol so both components read the single source
of truth.
- Around line 60-66: The empty-state renders the raw query in
SettingsSearchResults, which can be too long or contain problematic characters;
add a small helper (e.g., truncateQuery(query: string, max = 50): string) and
use it when rendering the <p> so the displayed string is shortened with an
ellipsis if over the limit (and continue to rely on React's text rendering for
escaping); update the JSX in SettingsSearchResults where it currently renders
`No settings found for "{query}"` to use the truncated/sanitized value (e.g.,
truncated = truncateQuery(query) and render `No settings found for
"{truncated}"`).
In `@src/test/renderer/settingsSearchIndex.test.ts`:
- Around line 20-27: Add an explicit failure message when misrouted integrations
are found: after computing misrouted (from INTEGRATION_REGISTRY and
SETTINGS_INDEX) add a guard like if (misrouted.length > 0) throw new
Error(`Misrouted integrations: ${misrouted.map(m => `${m.id} (expected
tabId=integrations)`).join(', ')}`) so CI shows the specific IDs and details
before the existing expect check in the test that routes entries to the
integrations tab.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d78976a1-f2d7-46bb-91fc-95a47aa76740
📒 Files selected for processing (22)
src/renderer/components/BrowserPreviewSettingsCard.tsxsrc/renderer/components/DefaultAgentSettingsCard.tsxsrc/renderer/components/HiddenToolsSettingsCard.tsxsrc/renderer/components/IntegrationsCard.tsxsrc/renderer/components/KeyboardSettingsCard.tsxsrc/renderer/components/NotificationSettingsCard.tsxsrc/renderer/components/RepositorySettingsCard.tsxsrc/renderer/components/ResourceMonitorSettingsCard.tsxsrc/renderer/components/ReviewAgentSettingsCard.tsxsrc/renderer/components/RightSidebarSettingsCard.tsxsrc/renderer/components/SettingsPage.tsxsrc/renderer/components/SettingsSearchInput.tsxsrc/renderer/components/SettingsSearchResults.tsxsrc/renderer/components/TaskHoverActionCard.tsxsrc/renderer/components/TaskSettingsRows.tsxsrc/renderer/components/TelemetryCard.tsxsrc/renderer/components/TerminalSettingsCard.tsxsrc/renderer/components/ThemeCard.tsxsrc/renderer/components/WorkspaceProviderInfoCard.tsxsrc/renderer/components/integrations/registry.tssrc/renderer/hooks/useSettingsSearch.tssrc/test/renderer/settingsSearchIndex.test.ts
| // Map setting IDs to their DOM element IDs for scrolling | ||
| export const SETTING_ELEMENT_IDS: Record<string, string> = { | ||
| // General | ||
| telemetry: 'telemetry-card', | ||
| 'auto-generate-task-names': 'auto-generate-task-names-row', | ||
| 'auto-infer-task-names': 'auto-infer-task-names-row', | ||
| 'auto-approve-by-default': 'auto-approve-by-default-row', | ||
| 'create-worktree-by-default': 'create-worktree-by-default-row', | ||
| 'auto-trust-worktrees': 'auto-trust-worktrees-row', | ||
| 'notifications-enabled': 'notification-settings-card', | ||
| 'notification-sound': 'notification-settings-card', | ||
| 'os-notifications': 'notification-settings-card', | ||
| 'sound-focus-mode': 'notification-settings-card', | ||
| 'sound-profile': 'notification-settings-card', | ||
| // Agents | ||
| 'default-agent': 'default-agent-settings-card', | ||
| 'review-agent': 'review-agent-settings-card', | ||
| 'review-prompt': 'review-agent-settings-card', | ||
| 'cli-agents': 'cli-agents-section', | ||
| // Integrations | ||
| integrations: 'integrations-card', | ||
| 'workspace-provider': 'workspace-provider-card', | ||
| // Repository | ||
| 'branch-prefix': 'repository-settings-card', | ||
| 'push-on-create': 'repository-settings-card', | ||
| 'auto-close-issues': 'repository-settings-card', | ||
| // Interface | ||
| theme: 'theme-card', | ||
| 'terminal-font-family': 'terminal-settings-card', | ||
| 'terminal-font-size': 'terminal-settings-card', | ||
| 'auto-copy-selection': 'terminal-settings-card', | ||
| 'mac-option-is-meta': 'terminal-settings-card', | ||
| 'keyboard-shortcuts': 'keyboard-settings-card', | ||
| 'auto-right-sidebar': 'right-sidebar-settings-card', | ||
| 'resource-monitor': 'resource-monitor-settings-card', | ||
| 'browser-preview': 'browser-preview-settings-card', | ||
| 'task-hover-action': 'task-hover-action-card', | ||
| 'hidden-tools': 'hidden-tools-settings-card', | ||
| }; |
There was a problem hiding this comment.
Missing element ID mappings for individual integration search results.
SETTINGS_INDEX (in useSettingsSearch.ts) includes entries for individual integrations (github, gitlab, jira, linear, plain, forgejo, sentry), but these IDs are not mapped in SETTING_ELEMENT_IDS. When a user clicks on a search result for "GitHub" or "Jira", the tab will switch to Integrations but no scroll will occur since elementId will be undefined.
Consider mapping all individual integration IDs to the integrations card:
🔧 Proposed fix to add missing integration mappings
// Integrations
integrations: 'integrations-card',
+ github: 'integrations-card',
+ gitlab: 'integrations-card',
+ jira: 'integrations-card',
+ linear: 'integrations-card',
+ plain: 'integrations-card',
+ forgejo: 'integrations-card',
+ sentry: 'integrations-card',
'workspace-provider': 'workspace-provider-card',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Map setting IDs to their DOM element IDs for scrolling | |
| export const SETTING_ELEMENT_IDS: Record<string, string> = { | |
| // General | |
| telemetry: 'telemetry-card', | |
| 'auto-generate-task-names': 'auto-generate-task-names-row', | |
| 'auto-infer-task-names': 'auto-infer-task-names-row', | |
| 'auto-approve-by-default': 'auto-approve-by-default-row', | |
| 'create-worktree-by-default': 'create-worktree-by-default-row', | |
| 'auto-trust-worktrees': 'auto-trust-worktrees-row', | |
| 'notifications-enabled': 'notification-settings-card', | |
| 'notification-sound': 'notification-settings-card', | |
| 'os-notifications': 'notification-settings-card', | |
| 'sound-focus-mode': 'notification-settings-card', | |
| 'sound-profile': 'notification-settings-card', | |
| // Agents | |
| 'default-agent': 'default-agent-settings-card', | |
| 'review-agent': 'review-agent-settings-card', | |
| 'review-prompt': 'review-agent-settings-card', | |
| 'cli-agents': 'cli-agents-section', | |
| // Integrations | |
| integrations: 'integrations-card', | |
| 'workspace-provider': 'workspace-provider-card', | |
| // Repository | |
| 'branch-prefix': 'repository-settings-card', | |
| 'push-on-create': 'repository-settings-card', | |
| 'auto-close-issues': 'repository-settings-card', | |
| // Interface | |
| theme: 'theme-card', | |
| 'terminal-font-family': 'terminal-settings-card', | |
| 'terminal-font-size': 'terminal-settings-card', | |
| 'auto-copy-selection': 'terminal-settings-card', | |
| 'mac-option-is-meta': 'terminal-settings-card', | |
| 'keyboard-shortcuts': 'keyboard-settings-card', | |
| 'auto-right-sidebar': 'right-sidebar-settings-card', | |
| 'resource-monitor': 'resource-monitor-settings-card', | |
| 'browser-preview': 'browser-preview-settings-card', | |
| 'task-hover-action': 'task-hover-action-card', | |
| 'hidden-tools': 'hidden-tools-settings-card', | |
| }; | |
| // Map setting IDs to their DOM element IDs for scrolling | |
| export const SETTING_ELEMENT_IDS: Record<string, string> = { | |
| // General | |
| telemetry: 'telemetry-card', | |
| 'auto-generate-task-names': 'auto-generate-task-names-row', | |
| 'auto-infer-task-names': 'auto-infer-task-names-row', | |
| 'auto-approve-by-default': 'auto-approve-by-default-row', | |
| 'create-worktree-by-default': 'create-worktree-by-default-row', | |
| 'auto-trust-worktrees': 'auto-trust-worktrees-row', | |
| 'notifications-enabled': 'notification-settings-card', | |
| 'notification-sound': 'notification-settings-card', | |
| 'os-notifications': 'notification-settings-card', | |
| 'sound-focus-mode': 'notification-settings-card', | |
| 'sound-profile': 'notification-settings-card', | |
| // Agents | |
| 'default-agent': 'default-agent-settings-card', | |
| 'review-agent': 'review-agent-settings-card', | |
| 'review-prompt': 'review-agent-settings-card', | |
| 'cli-agents': 'cli-agents-section', | |
| // Integrations | |
| integrations: 'integrations-card', | |
| github: 'integrations-card', | |
| gitlab: 'integrations-card', | |
| jira: 'integrations-card', | |
| linear: 'integrations-card', | |
| plain: 'integrations-card', | |
| forgejo: 'integrations-card', | |
| sentry: 'integrations-card', | |
| 'workspace-provider': 'workspace-provider-card', | |
| // Repository | |
| 'branch-prefix': 'repository-settings-card', | |
| 'push-on-create': 'repository-settings-card', | |
| 'auto-close-issues': 'repository-settings-card', | |
| // Interface | |
| theme: 'theme-card', | |
| 'terminal-font-family': 'terminal-settings-card', | |
| 'terminal-font-size': 'terminal-settings-card', | |
| 'auto-copy-selection': 'terminal-settings-card', | |
| 'mac-option-is-meta': 'terminal-settings-card', | |
| 'keyboard-shortcuts': 'keyboard-settings-card', | |
| 'auto-right-sidebar': 'right-sidebar-settings-card', | |
| 'resource-monitor': 'resource-monitor-settings-card', | |
| 'browser-preview': 'browser-preview-settings-card', | |
| 'task-hover-action': 'task-hover-action-card', | |
| 'hidden-tools': 'hidden-tools-settings-card', | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsPage.tsx` around lines 101 - 139,
SETTINGS_INDEX contains individual integration IDs (github, gitlab, jira,
linear, plain, forgejo, sentry) but SETTING_ELEMENT_IDS lacks mappings for those
keys, so clicking an integration search result switches to the Integrations tab
without scrolling; update SETTING_ELEMENT_IDS to include entries for "github",
"gitlab", "jira", "linear", "plain", "forgejo", and "sentry" and map each to
'integrations-card' so the useSettingsSearch scroll logic finds a valid
elementId. Ensure you modify the exported SETTING_ELEMENT_IDS constant in
SettingsPage.tsx to add these keys.
Move elementId from a separate SETTING_ELEMENT_IDS map in SettingsPage into the SearchableSetting type as a required field. Single source of truth, and TypeScript now enforces that every searchable setting has a scroll target.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
lol didnt know deleting my forked repo will also completely destroy this PR. will make a new pr asap lol |
Summary
finally able to search for the setting i was looking for quicker lol
very open on the design btw but its ok for testing this out for now
maybe we should make this just a search icon and when you click on it it expands(???) but then again i thought that would be too hard to maybe notice idk lol
this in general could also be a lil overcomplicated maybe but i really wanted to be able to search for e.g. jira or ticket and find jira and stuff
Fixes
#1682
Snapshot
Screen.Recording.2026-04-07.at.10.57.33.mov
Type of change
Mandatory Tasks
Checklist
Summary by CodeRabbit